Preview course materials - #14907
Conversation
github-actions
Bot
added
APP: Coach
Build Artifacts
Smoke test screenshot |
68cc693 to
012c1c4
Compare
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
Added just a couple of notes
| // Side panel routes accessible from the course summary page | ||
| COURSE_SUMMARY_ASSIGN: 'COURSE_SUMMARY_ASSIGN', | ||
| COURSE_SUMMARY_ASSIGN_COURSE_DETAILS: 'COURSE_SUMMARY_ASSIGN_COURSE_DETAILS', | ||
| COURSE_SUMMARY_ASSIGN_COURSE_PREVIEW: 'COURSE_SUMMARY_ASSIGN_COURSE_PREVIEW', |
There was a problem hiding this comment.
This might be a bit redundant? but following the existing pattern for course routing as I suspect there was good reason for it :)
🔵 Review postedLast updated: 2026-07-29 22:10 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 adds a browsable course-resource preview side panel. Implementation is clean and matches the linked scope; the tree navigation (remount :key, _topicFetchId stale-guard, no-trust-of-topic.value-while-loading) is carefully reasoned. Findings below are all non-blocking.
CI passing. Manual QA was required for these UI changes but did not run in this pipeline, so the rendered UI has not been visually verified — not approving on UI grounds.
- suggestion — New logic-bearing components ship with no tests; the
goBack/route-derivation state machine is the part most likely to regress silently (inline). - suggestion —
CourseResourcePreview:label="contentNode.kind"is dead and would be an untranslated enum if rendered (inline). - suggestion — Shared
SidePanelModalheader tweak is unrelated to this feature and silently affects the search-filters back state (inline). - suggestion — Topic title (content text) lacks
dir="auto"for RTL (inline). - nitpick — Back during an in-flight topic fetch exits the whole preview (inline).
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
|
Hi @marcellamaki, Noticed the following issues:
video.with.description.mp4
bookmarks.mp4Improvement suggestion:
whynobreadcrumb.mp4 |
|
Thank you @pcenov -- I'll address these! The video bit is particularly strange. Good find 😂 |
012c1c4 to
eceb429
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review. 1 of 5 prior findings resolved (dir="auto" on the topic title); 4 still open (all suggestion/nitpick). No new findings. CI pending; manual QA was required but did not run, so the UI is not visually verified.
Still-open findings (see inline comments):
- CoursePreviewSidePanel.vue:226 — no
*.spec.jsforgoBack's three paths and the stale-loading fallback;views/courses/__tests__/already has the pattern. - CourseResourcePreview.vue:7 — dead
:labelbinding on a raw enum; drop it. - SidePanelModal/index.vue:33 — shared-component style change is unrelated to this flow; it only affects
SearchFiltersPanel's back state. - CoursePreviewSidePanel.vue:234 — nitpick: back during in-flight fetch exits preview rather than going up a level (documented as deliberate).
Prior-finding status
RESOLVED — CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
UNADDRESSED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation
UNADDRESSED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview
UNADDRESSED — CourseResourcePreview.vue:7 — dead :label binding on raw enum
UNADDRESSED — SidePanelModal/index.vue:33 — unrelated shared-component change
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
eceb429 to
d1eb69a
Compare
github-actions
Bot
added
DEV: renderers
d1eb69a to
d7ba770
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review. All five prior findings resolved (dir="auto" persists, spec covers the three goBack paths + stale-loading fallback, dead :label replaced by KLabeledIcon+title, the unrelated SidePanelModal change is gone, loading fallback now hides the back button rather than exiting preview). One new edge-case suggestion below on the error path; not blocking.
CI pending. Manual QA was required for these UI changes but did not run this pass — the rendered UI is not visually verified, so this is not an approval on UI grounds.
- suggestion —
goBackHandlercan dereference a nulltopicon the fetch-error path (see inline).
Prior-finding status
RESOLVED — CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
RESOLVED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation
RESOLVED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview
RESOLVED — CourseResourcePreview.vue:7 — dead :label binding on raw enum
RESOLVED — SidePanelModal/index.vue:33 — unrelated shared-component change
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
Course details workflow:
Potential issue with direct navigation to a bookmarked page (preview of a single resource), "back" button apparently moves well up the tree with one difference (screenshot below):
Maybe a question for @jtamiace?
I couldn't replicate this with any Flexbox resources included in the 2 courses. Will keep an 👁️ Of the 2 issues reported by @pcenov, bookmarking resources during preview is now working correctly ✔️, but it is still not possible to use the back ⬅️ button if the video has a description (one must first press |
d7ba770 to
edaa67d
Compare
|
Per slack chat with @radinamatic and @rtibbles, we are going to just remove the bookmark functionality all together (not required for previewing, adds more headaches than it's probably worth at least for now). I also have double-double checked the video preview back button and really think it's fixed so hopefully QA team agrees with me 😂 🤞 ready for re-review -- thank you! |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review (d7ba770 → edaa67d). 9 of 10 prior findings resolved; 1 still open (see inline). Removing bookmarking from the preview panel is a clean scope reduction, and it came with a test that renders the real AccessibleResourceCard.
CI still pending (only Python/Morango jobs outstanding — none touch this diff); frontend tests and lint pass. Manual QA did not run, so nothing here is visually verified.
New, both non-blocking and inline: the shared SlotTruncator CSS fix looks correct but silently changes overlay geometry for three consumers outside this feature, and :showBookmarkButton is bound on the folder-card branch of <component :is> where the prop doesn't exist.
Prior-finding status
RESOLVED — CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
RESOLVED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation (round 1)
RESOLVED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation (round 2)
RESOLVED — CoursePreviewSidePanel.vue:180 — branching route derivation untested/duplicated
RESOLVED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview (round 1)
RESOLVED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview (round 2)
RESOLVED — CourseResourcePreview.vue:7 — dead :label binding on raw enum (round 1)
RESOLVED — CourseResourcePreview.vue:7 — dead :label binding on raw enum (round 2)
RESOLVED — SidePanelModal/index.vue:33 — unrelated shared-component change
UNADDRESSED — CoursePreviewSidePanel.vue:171 — goBackHandler dereferences null topic on the fetch-error path
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
Hi @marcellamaki - already looks great and I confirm that the rogue For the breadcrumb navigation I was wondering whether it would be possible to add a direct link to the actual main navigation.mp4Also observed the following issue: I'm sometimes getting an error when I go to a error.mp4Console: |
yes, that makes sense to me, @pcenov. I think it would work well here, and on the "course details" version of this (meaning, when the course is already assigned), I will still skip that breadcrumb (as it wouldn't make sense in that context, I don't think) As for the kpub - I am curious if this is an underlying bug with something about the kpub state management... I'll investigate |
edaa67d to
6a17c30
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review of the new "Courses" breadcrumb crumb and its two tests (edaa67d..6a17c30).
9 of 12 prior findings resolved; 3 still open (one substantive, two acknowledged as no-action).
The crumb is correctly gated on the assign-vs-summary flow, using the same courseSessionId discriminator exitPreview() already relies on. New findings are all minor and inline. CI is still pending (outstanding checks are Python/Morango jobs that don't touch this diff); manual QA did not run in this pass, so nothing here is visually verified.
Still open from earlier rounds:
- CoursePreviewSidePanel.vue:185 — suggestion:
goBackguards the loading window but not the error window.useFetchleavestopicuntouched and setsloading=falseon rejection, so a failed firstfetchTreeon a deep-topic URL leaves a blank panel with a live back button that throws ontopic.value.parent. Third round unaddressed and not rebutted — still non-blocking; a one-line reply saying it's intentional would close it out.
Prior-finding status
UNADDRESSED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:185 — goBackHandler dereferences null topic on the fetch-error path
ACKNOWLEDGED — packages/kolibri-common/components/SlotTruncator.vue:113 — shared CSS fix changes overlay geometry for three consumers outside this feature
ACKNOWLEDGED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:45 — :showBookmarkButton bound on the folder-card branch of <component :is>
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:180 — branching route derivation untested/duplicated
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CourseResourcePreview.vue:7 — dead :label binding on raw enum
RESOLVED — packages/kolibri-common/components/SidePanelModal/index.vue:33 — unrelated shared-component change
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
6a17c30 to
9b0bbec
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review of 6a17c30..9b0bbec.
11 of 11 prior findings settled (9 resolved, 2 acknowledged); 2 new non-blocking findings below. CI is green on 9b0bbec (86/86). Manual QA did not run in this pass — nothing was visually verified.
- suggestion — the back-button guard keys off
useFetch's sharederrorref, so a failed "View more" hides the back button on an otherwise fully-loaded topic (inline). - nitpick — the fetch-error state still renders as an empty panel (inline).
Prior-finding status
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:185 — goBackHandler dereferences null topic on the fetch-error path
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/tests/CoursePreviewSidePanel.spec.js:88 — crumb test doesn't assert the destination
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:32 — redundant single crumb in the summary flow
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:32 — coursesLabel$() vs selectCourseLabel$() for the crumb
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:226 — no spec for goBack / route derivation
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:234 — loading fallback drops the user out of preview
RESOLVED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CourseResourcePreview.vue:7 — dead :label binding on raw enum
RESOLVED — packages/kolibri-common/components/SidePanelModal/index.vue:33 — unrelated shared-component change
ACKNOWLEDGED — packages/kolibri-common/components/SlotTruncator.vue:113 — shared CSS fix changes overlay geometry for three consumers outside this feature
ACKNOWLEDGED — kolibri/plugins/coach/frontend/views/courses/sidePanels/AssignCourse/subpages/CoursePreviewSidePanel.vue:45 — :showBookmarkButton bound on the folder-card branch of <component :is>
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
|
||
| <template v-else> | ||
| <KCircularLoader v-if="loading" /> | ||
| <div v-else-if="children"> |
There was a problem hiding this comment.
nitpick: Consuming error closed the crash, but nothing renders it: on a rejected first fetchTree, loading is false and children is null, so this branch is false and the body is empty — and the back button is now hidden too, leaving SidePanelModal's close/backdrop as the only exit. CourseUnitView/index.vue:954 has the in-repo one-liner (watch(error, ... handleApiError({ error }))) if you want it. Flagging only because the guard fix makes the blank state slightly more of a dead end than before.
Implement rtibblesbot cleanup fixes
9b0bbec to
6e821b9
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review of 9b0bbec → 6e821b9 (7 added / 6 removed lines, all in CoursePreviewSidePanel.vue).
11 of 14 prior findings resolved; 2 acknowledged, 1 still open (see inline). One new nitpick on how the narrowed back-button guard was wrapped — nothing blocking.
The guard now keys on topic.value?.id !== currentTopicId.value instead of loading || error, which is the right invariant: a failed "View more" on a fully-rendered topic no longer hides the back button, and all three stale/missing-topic cases stay covered by the existing specs.
CI: frontend tests, lint, CodeQL and Python 3.10 unit tests green on 6e821b9; remaining jobs are packaging/Android/Morango, none of which touch this frontend-only diff. Manual QA did not run in this pass, so nothing here is a statement about the rendered UI.
Prior-finding status
RESOLVED — CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
RESOLVED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation
RESOLVED — CoursePreviewSidePanel.vue:180 — branching route derivation untested/duplicated
RESOLVED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview
RESOLVED — CourseResourcePreview.vue:7 — dead :label binding on raw enum
RESOLVED — SidePanelModal/index.vue:33 — unrelated shared-component change
RESOLVED — CoursePreviewSidePanel.vue:185 — goBackHandler dereferences null topic on the fetch-error path
RESOLVED — CoursePreviewSidePanel.vue:32 — redundant single breadcrumb at course root
RESOLVED — CoursePreviewSidePanel.vue:147 — crumb label ambiguous (coursesLabel$ → selectCourseLabel$)
RESOLVED — tests/CoursePreviewSidePanel.spec.js:91 — crumb test asserted text, not destination
RESOLVED — CoursePreviewSidePanel.vue:199 — isAmbiguousParent conflated fetchMore errors with an ambiguous parent
ACKNOWLEDGED — SlotTruncator.vue:113 — shared CSS fix affects three consumers outside this feature
ACKNOWLEDGED — CoursePreviewSidePanel.vue:45 — :showBookmarkButton bound on the folder-card branch
UNADDRESSED — CoursePreviewSidePanel.vue:19 — fetch error is never surfaced
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| // or if the fetch fails, it may still hold stale data (or none at all). Hide | ||
| // the back button in both cases rather than navigating to a wrong/missing parent. | ||
| const goBack = computed(() => { | ||
| const isAmbiguousParent = computed( |
There was a problem hiding this comment.
nitpick: The inner computed() should be a plain expression — it buys nothing over the boolean and leaks a watcher per evaluation.
const goBack = computed(() => {
const isAmbiguousParent =
!previewContentId.value &&
currentTopicId.value !== route.params.courseId &&
topic.value?.id !== currentTopicId.value;
return isAmbiguousParent ? null : goBackHandler;
});Vue 2.7.16's computed() constructs new Watcher(vm, getter, noop, { lazy: true }) (vue/dist/vue.runtime.common.dev.js:1312), whose constructor calls recordEffectScope(this, … vm._scope …) (:3352-3360) — pushing the watcher into the component's effect scope, released only on destroy. Computed getters run inside _render where setCurrentInstance(vm) is active (:2600), so every re-evaluation of goBack (each route/topic change) appends another never-collected lazy watcher on the same deps. Bounded by the panel's lifetime and not user-visible, so purely cleanup.
|
|
||
| <template v-else> | ||
| <KCircularLoader v-if="loading" /> | ||
| <div v-else-if="children"> |
There was a problem hiding this comment.
nitpick: Still open from the last round, raised for the record. error is no longer destructured from useFetch at all, so a rejected first fetchTree leaves loading false and children null — the panel body renders empty with the back button hidden and no way to retry or understand why. A v-else branch with an error message (and ideally a retry) would close it.
marcellamaki
changed the title
5 participants
Summary
After some slack discussion with Richard and Alex, this implements a "course content previewing" option based on the existing side panel lesson and quiz content previewing/selection experience. Within a course, specific subsections of content cannot be selected, thus no checkboxes -- it's just about seeing the content and being able to access everything a learner will see. Pre-post test assessments are not included.
References
Fixes #14149
Screen.Recording.2026-07-14.at.6.56.02.PM.mov
Reviewer guidance
Assign course workflow AND Course details workflow should be tested (although they share components, so it's unlikely that there would be significant UI differences but there might be some data bugs I suppose!)
one extra manual QA note: I had one instance on load where flexbook content didn't load and I didn't have my console open for an error 🤦♀️ . I haven't been able to reproduce it. Worth seeing if this pops up.
AI usage
Implementation done collaboratively with Claude (with moments for human in the loop feedback on the workflow from both me and Richard via slack discussions to do some decision making). Two separate rounds of human/self-review of the code. 1 upon initial completion, and now 1 ~2 weeks later when marking issue as ready for review.